I can see the logic behind putting the loop in MarkAllUpdatesToolButton_Click and having the single mark for update code in MainModList. It makes the MarkModForUpdate method more reusable, and puts the logic of "Mark all available updates" where it arguably should be (attached to the button that the user clicks to do that).

On the other hand, it's looping inside a loop, which is inefficient. Do we need to loop across the entire gridview to find a specific identifier in ModList? Is there a way to directly access the correct row? Some kind of LINQ query?

I think it you want to put the "Mark all available updates" looping code into MainModList (where it may arguably belong as an action taken on the ModList) it would be ideal to have it as a separate method. Simply putting the loop from MarkAllUpdatesToolButton_Click into MainModList maintains the nested loop, so that would be redundant. Your new single loop method is much more efficient. If we were calling MarkModForUpdate from anywhere else, it would be an easy call. It doesn't seem rational to leave the old code there if it isn't used.

Is there a way to rescue MarkModForUpdate by removing its loop?

Let me be clear, I'm entirely cool with that answer being "no", I'm just spilling out what's going on in my brain. There are many times with this code where I find myself thinking that the AwesomeCodersWhoCameBefore must have had Reasons for the choices they made. Those Reasons aren't necessarily right, though. 😃

I'm not sure if I can follow you exactly.
You want to split MarkModsForUpdate to split up in two methods, one MarkAllModsForUpdate (containing the loop/ query) and oneMarkSingleModForUpdate (just selecting a single mod, maybe called inside MarkAllModsForUpdate ?
That may be a good idea.

There is a map from identifier => row, see MarkModForInstall:

That could be used to eliminate the loop in the old code if we do keep MarkModForUpdate.

So before your changes, it was split up . You've basically merged the two elements (Find all mods which have updates) fromMarkAllUpdatesToolButton_Click and (Change the update checkbox) from MarkModForUpdate. If we change MarkModForUpdate to get the relevant row using DataGridViewRow row = mainModList.full_list_of_mod_rows[identifier]; then we get rid of the inefficiency and keep the concept of a method doing a single conceptual task.

I'm assuming that refreshing the modlist will cause the status of the Apply button to change?

So like this now?
I dont't know what row.Cells[1] is DataGridViewCheckBoxCell is/was for, this just checks if the first cell of the row is a checkbox?

It's testing to see if the second column (UpdateCol) is a checkbox. You might want to check what makes the status of that column change. I would have thought that mod.HasUpdate includes the same checks, so it may be redundant.

SetUpgradeChecked has a comment, //Contract.Requires<ArgumentException>(row.Cells[1] is DataGridViewCheckBoxCell); So checking for that might be to avoid a type mismatch exception? I don't know why the Contract.Requires is commented out.

I got it:
When we initiate all the cells in ConstructModList(), the cell updating (= row.Cells[1]) gets either declared as

• DataGridViewCheckboxCell and casted to DataGridViewCell if the mod is installed, or
• DataGridViewTextBoxCell if the mod is not installed:

var updating = mod.IsInstallable() && mod.HasUpdate
    ? (DataGridViewCell) new DataGridViewCheckBoxCell() {
        Value = myChange == null ? false
            : myChange.ChangeType == GUIModChangeType.Update ? true
            : false
     } : new DataGridViewTextBoxCell() {
         Value    = "-"
     };

But because GUIMos.HasUpdate = registry.HasUpdate() which checks if the mod is installed, the type check should be redundant:

/// <summary>
/// Is the mod installed and does it have a newer version compatible with version
/// We can't update AD mods
/// </summary>
public static bool HasUpdate(this IRegistryQuerier querier, string identifier, KspVersionCriteria version)
{
    CkanModule newest_version;
    try
    {
        newest_version = querier.LatestAvailable(identifier, version);
    }
    catch (ModuleNotFoundKraken)
    {
        return false;
    }
    if (newest_version == null) return false;
    return !new List<string>(querier.InstalledDlls).Contains(identifier)
        && querier.IsInstalled(identifier, false)
        && newest_version.version.IsGreaterThan(querier.InstalledVersion(identifier));
}

Cool. This should make for a good little speedup.

Ah, forgot to remove the comments...
We'll, may the next one tumbling over those lines worry his brain...